refactor: move bsearch function to C-code #2945

eloycoto · 2023-11-15T15:52:25Z

This commit fixes issue #527 and move the bsearch function to a native C-code.

The performance is a bit better:

Testing script:

clear
if [[ `uname` == Darwin ]]; then
    MAX_MEMORY_UNITS=KB
else
    MAX_MEMORY_UNITS=MB
fi

export TIMEFMT='%J   %U  user %S system %P cpu %*E total'$'\n'\
'avg shared (code):         %X KB'$'\n'\
'avg unshared (data/stack): %D KB'$'\n'\
'total (sum):               %K KB'$'\n'\
'max memory:                %M '$MAX_MEMORY_UNITS''$'\n'\
'page faults from disk:     %F'$'\n'\
'other page faults:         %R'

echo "JQ code bsearch"
time /usr/bin/jq -n '[range(30000000)] | bsearch(3000)'

echo "C code bsearch"
time ./jq -n '[range(30000000)] | bsearch(3000)'

Results:

JQ code bsearch
3000
/usr/bin/jq -n '[range(30000000)] | bsearch(3000)'   8.63s  user 0.77s system 98% cpu 9.542 total
avg shared (code):         0 KB
avg unshared (data/stack): 0 KB
total (sum):               0 KB
max memory:                823 MB
page faults from disk:     1
other page faults:         432828
C code bsearch
3000
./jq -n '[range(30000000)] | bsearch(3000)'   8.44s  user 0.74s system 99% cpu 9.249 total
avg shared (code):         0 KB
avg unshared (data/stack): 0 KB
total (sum):               0 KB
max memory:                824 MB
page faults from disk:     0
other page faults:         432766

The results may be better if we can use jvp_array_read, and there is no need to copy/free the input array in each iteration. I guess that is like that for API pourposes when the libjq is in use with multiple threads in place.

Closes #527

src/builtin.c

eloycoto · 2023-11-21T10:09:33Z

@emanuele6 I think this is ready

itchyny · 2023-12-04T14:41:27Z

The bsearch filter should support searching from an array of any kind.

 $ jq -n '[{x:range(3),y:range(3)}] | debug(. == sort) | bsearch({x:1,y:1})'
["DEBUG:",true]
4

eloycoto · 2024-02-05T08:33:29Z

@emanuele6 @itchyny I've just refactored this, and merging should be okay.

src/builtin.c

tests/jq.test

itchyny · 2024-02-07T11:08:52Z

I found a regression. On updating tests see my review comment above.

 $ jq -n '[1,2] | bsearch(2)'          
1
 $ ./jq -n '[1,2] | bsearch(2)'
-3

itchyny · 2024-02-12T01:23:59Z

I think we can reduce special cases, simplify the logic.

static jv f_bsearch(jq_state *jq, jv input, jv target) {
  if (jv_get_kind(input) != JV_KIND_ARRAY) {
    return type_error(input, "cannot be searched from");
  }
  int start = 0;
  int end = jv_array_length(jv_copy(input));
  jv answer = jv_invalid();
  while (start < end) {
    int mid = (start + end) / 2;
    int result = jv_cmp(jv_copy(target), jv_array_get(jv_copy(input), mid));
    if (result == 0) {
      answer = jv_number(mid);
      break;
    } else if (result < 0) {
      end = mid;
    } else {
      start = mid + 1;
    }
  }
  if (!jv_is_valid(answer)) {
    answer = jv_number(-1 - start);
  }
  jv_free(input);
  jv_free(target);
  return answer;
}

emanuele6 · 2024-02-19T23:41:51Z

src/builtin.c

+/* If the input is not sorted, bsearch will terminate but with irrelevant results. */
+static jv f_bsearch(jq_state *jq, jv input, jv target) {
+  if (jv_get_kind(input) != JV_KIND_ARRAY) {
+    return type_error(input, "cannot be searched from");


Here you are leaking target; you need to free it before returning.
Otherwise looks good to me.

Maybe also add a test with a non-array input, and a target that uses allocated memory (e.g. a string), so this can be caught by the CI?

emanuele6 · 2024-02-20T00:11:57Z

I just remembered that (start + end) / 2 for binary search is not recommended because it can overflow.
This can be avoided using start + (end - start) / 2 instead.
See https://en.wikipedia.org/wiki/Binary_search_algorithm#Implementation_issues

tests/jq.test

src/builtin.c

This commit fixes issue jqlang#527 and move the bsearch function to a native C-code. The performance is a bit better: Testing script: ```bash clear if [[ `uname` == Darwin ]]; then MAX_MEMORY_UNITS=KB else MAX_MEMORY_UNITS=MB fi export TIMEFMT='%J %U user %S system %P cpu %*E total'$'\n'\ 'avg shared (code): %X KB'$'\n'\ 'avg unshared (data/stack): %D KB'$'\n'\ 'total (sum): %K KB'$'\n'\ 'max memory: %M '$MAX_MEMORY_UNITS''$'\n'\ 'page faults from disk: %F'$'\n'\ 'other page faults: %R' echo "JQ code bsearch" time /usr/bin/jq -n '[range(30000000)] | bsearch(3000)' echo "C code bsearch" time ./jq -n '[range(30000000)] | bsearch(3000)' ```` Results: ``` JQ code bsearch 3000 /usr/bin/jq -n '[range(30000000)] | bsearch(3000)' 8.63s user 0.77s system 98% cpu 9.542 total avg shared (code): 0 KB avg unshared (data/stack): 0 KB total (sum): 0 KB max memory: 823 MB page faults from disk: 1 other page faults: 432828 C code bsearch 3000 ./jq -n '[range(30000000)] | bsearch(3000)' 8.44s user 0.74s system 99% cpu 9.249 total avg shared (code): 0 KB avg unshared (data/stack): 0 KB total (sum): 0 KB max memory: 824 MB page faults from disk: 0 other page faults: 432766 ``` The results may be better if we can use jvp_array_read, and there is no need to copy/free the input array in each iteration. I guess that is like that for API pourposes when the libjq is in use with multiple threads in place. Signed-off-by: Eloy Coto <[email protected]>

Co-authored-by: itchyny <[email protected]> Signed-off-by: Eloy Coto <[email protected]>

Signed-off-by: Eloy Coto <[email protected]>

.

eloycoto force-pushed the BSearchCCode branch 2 times, most recently from d975a4c to 9e3c7bc Compare November 15, 2023 16:23

emanuele6 reviewed Nov 15, 2023

View reviewed changes

src/builtin.c Outdated Show resolved Hide resolved

emanuele6 reviewed Nov 15, 2023

View reviewed changes

src/builtin.c Outdated Show resolved Hide resolved

eloycoto force-pushed the BSearchCCode branch from 9e3c7bc to e045979 Compare November 15, 2023 21:25

eloycoto changed the title ~~refactor: move bsearch function to C-code~~ WIP refactor: move bsearch function to C-code Nov 15, 2023

emanuele6 reviewed Nov 15, 2023

View reviewed changes

src/builtin.c Outdated Show resolved Hide resolved

src/builtin.c Outdated Show resolved Hide resolved

src/builtin.c Outdated Show resolved Hide resolved

src/builtin.c Outdated Show resolved Hide resolved

itchyny added the performance label Nov 16, 2023

eloycoto force-pushed the BSearchCCode branch 2 times, most recently from 29c1f4e to 618c10f Compare November 16, 2023 10:43

eloycoto changed the title ~~WIP refactor: move bsearch function to C-code~~ refactor: move bsearch function to C-code Nov 16, 2023

emanuele6 requested a review from nicowilliams November 21, 2023 12:02

suisanT1 approved these changes Dec 27, 2023

View reviewed changes

eloycoto force-pushed the BSearchCCode branch 2 times, most recently from 4866769 to 949b647 Compare February 5, 2024 08:17

itchyny reviewed Feb 5, 2024

View reviewed changes

src/builtin.c Outdated Show resolved Hide resolved

itchyny reviewed Feb 5, 2024

View reviewed changes

src/builtin.c Outdated Show resolved Hide resolved

itchyny reviewed Feb 5, 2024

View reviewed changes

src/builtin.c Outdated Show resolved Hide resolved

itchyny reviewed Feb 5, 2024

View reviewed changes

src/builtin.c Outdated Show resolved Hide resolved

eloycoto force-pushed the BSearchCCode branch from 50fc13a to 569da27 Compare February 5, 2024 09:48

itchyny reviewed Feb 7, 2024

View reviewed changes

tests/jq.test Show resolved Hide resolved

eloycoto force-pushed the BSearchCCode branch 2 times, most recently from a77f02a to 42135ed Compare February 10, 2024 12:38

eloycoto force-pushed the BSearchCCode branch from 42135ed to 95cbcc5 Compare February 14, 2024 08:56

eloycoto requested a review from itchyny February 14, 2024 12:51

itchyny approved these changes Feb 19, 2024

View reviewed changes

emanuele6 previously requested changes Feb 19, 2024

View reviewed changes

eloycoto force-pushed the BSearchCCode branch 4 times, most recently from 359f0e2 to 2667c55 Compare February 29, 2024 09:58

emanuele6 reviewed Feb 29, 2024

View reviewed changes

tests/jq.test Outdated Show resolved Hide resolved

emanuele6 reviewed Feb 29, 2024

View reviewed changes

src/builtin.c Outdated Show resolved Hide resolved

eloycoto force-pushed the BSearchCCode branch from 2667c55 to df31443 Compare February 29, 2024 11:23

eloycoto and others added 3 commits February 29, 2024 12:24

style: fixes from suggestions.

097fc63

Co-authored-by: itchyny <[email protected]> Signed-off-by: Eloy Coto <[email protected]>

fix: bsearch prevent overflow on mid calculation

5c0aaba

Signed-off-by: Eloy Coto <[email protected]>

eloycoto force-pushed the BSearchCCode branch from df31443 to 5c0aaba Compare February 29, 2024 11:25

itchyny approved these changes Mar 6, 2024

View reviewed changes

itchyny added this to the 1.8 release milestone Mar 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: move bsearch function to C-code #2945

refactor: move bsearch function to C-code #2945

eloycoto commented Nov 15, 2023 •

edited by emanuele6

Loading

eloycoto commented Nov 21, 2023

itchyny commented Dec 4, 2023

eloycoto commented Feb 5, 2024

itchyny commented Feb 7, 2024

itchyny commented Feb 12, 2024 •

edited

Loading

emanuele6 Feb 19, 2024 •

edited

Loading

emanuele6 Feb 19, 2024

emanuele6 commented Feb 20, 2024 •

edited

Loading

refactor: move bsearch function to C-code #2945

Are you sure you want to change the base?

refactor: move bsearch function to C-code #2945

Conversation

eloycoto commented Nov 15, 2023 • edited by emanuele6 Loading

eloycoto commented Nov 21, 2023

itchyny commented Dec 4, 2023

eloycoto commented Feb 5, 2024

itchyny commented Feb 7, 2024

itchyny commented Feb 12, 2024 • edited Loading

emanuele6 Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

emanuele6 Feb 19, 2024

Choose a reason for hiding this comment

emanuele6 commented Feb 20, 2024 • edited Loading

eloycoto commented Nov 15, 2023 •

edited by emanuele6

Loading

itchyny commented Feb 12, 2024 •

edited

Loading

emanuele6 Feb 19, 2024 •

edited

Loading

emanuele6 commented Feb 20, 2024 •

edited

Loading